Performance: Try memoing large hotspots #32469
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +84 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
Performance job results 175937f Second run with the test fix 204f10e b117827 (memo style from useBlockProps and a few paragraph handler functions) |
|
Thank you for keeping on this, it's such critical path. I'm not sure how much increase I'm supposed to see, but I feel like this might be a big step forward, here's my experience:
Just regular typing now feels good. As suggested in the GIF, I can still do the "mash a ton of keys" to achieve a throttle effect. Still, this feels better to me! |
|
Thanks for looking @jasmussen. I'm glad to see you're feeling some improvement!
This is better left for other PRs, but in order to fully remove typing jank while working with larger documents we'll likely need to experiment with a few perceived performance optimizations. One technique for example might defer updating the post content state with debounce or throttle. So instead of triggering a rerender on each keypress we might wait for the user to stop typing, or only do so every X interval. Another more complicated optimization might be something like not rendering blocks that are not visible in the viewport (windowing). We can of course further profile the Buttons block and Navigation block too. Navigation links in particular has a potential improvement to not render submenus unless they are toggled, but I need to double check what dragging functionality we support). |
gwwar
force-pushed
the
try/experimental-memo
branch
from
b117827 to
6c02a88
Compare
kevin940726
reviewed
Jun 8, 2021
packages/block-editor/src/components/block-list/use-block-props/index.js
Outdated
Show resolved
Hide resolved
|
Two todos from the core-js meeting https://wordpress.slack.com/archives/C5UNMSU4R/p1623164420080800
I should also double check that performance hotspots are the same in the production build: eg |
|
I ran out of time for today, but I was trying to narrow down other leads for why keypress take so long. Looking further into the onInput handlers created in
|
|
Okay looking deeper with the profiling tab in trunk, here's a scenario where I'm tying a few letters for the first time at a steady pace: The start Typing Handler does double the first keypress task time, but we’re already over budget on normal key presses. Tasks should complete in 16.7ms for 60fps or 33.3ms for 30fps. Long tasks make the UI feel sluggish or appear to make the browser freeze.
Digging into the breakdown, most of that time spent is giving updates to global listeners. There are a few spots that we might be able to try and optimize (but has a high chance of breaking things), but I suspect we may need to defer dispatching state updates on every keypress. So I can try spinning up a seperate PR where we try a debounce strategy for keypress.
cc @youknowriad |
|
As another data-point, I experimented to see what performance impact it had if we no longer added the |
|
For what it's worth, the is-typing class exists primarily (only?) to allow us to fade out the toolbar when you are actually typing, so as to get out of your way. I also think we can be a fair bit more aggressive with this behavior, in case that opens up some opportunities for improvement. |
We already have that, basically what we defer is the subscribe calls, not the dispatch. Only the visible blocks are "synchronous", the remaining blocks are not. More details here https://riad.blog/2020/02/14/a-journey-towards-a-performant-web-editor/ (It used to be just the selected block, but we changed recently to make all the visible block sync to solve some issues without much impact on performance)
Unfortunately, I believe this is not a good solution, there's a lot of things in the block editor that require the selection and change to be synchronous (dirty checking also comes in mind). Very early in Gutenberg days, we tried this and ended up reverting. I think you could try but I wanted to share that it's also something we tried already. cc @ellatrix would know more here. |
The is-typing class seem to be used in just a single place in our styles, so we could potentially remove it (and find an alternative, like adding it exactly where we need it) like you did and avoid memoizing (which has its costs) |
Sure I can reopen and polish that one 👍 |
gwwar
force-pushed
the
try/experimental-memo
branch
from
9066a9b to
fa34895
Compare
|
I rebased with trunk after landing #32567. Here are the perf results: So it looks like |
@youknowriad (or anyone else) — happen to know what PR this was done in? This sounds like a great change that would solve some issues I'd encountered before! |
|
@chrisvanpatten sure #30995 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an experimental branch to display performance effects if we memo key hotspot components.
Looking at performance job results, we're able to improve
maxType, andmaxInserterOpenThe performance test case here involves using:
The goal here is to reduce wasted renders. For this particular test case I'd expect paragraph and related items to rerender, not so much the header chrome or sibling blocks for each keypress. Each item I chose to memo showed up in the react profiler, and I also verified that memo works with the default shallow comparison (eg there isn't a caller that's always passing a new prop).
Testing Instructions
Before:
profiletrunk.mp4
After:
branchprofile.mp4